-
Notifications
You must be signed in to change notification settings - Fork 277
Value set: lift offset from numeric constants to expressions #8647
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
9764fbe
to
5d136c4
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8647 +/- ##
===========================================
- Coverage 80.36% 80.36% -0.01%
===========================================
Files 1688 1688
Lines 207067 207073 +6
Branches 73 73
===========================================
- Hits 166418 166414 -4
- Misses 40649 40659 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
5d136c4
to
41811ba
Compare
@@ -981,7 +981,7 @@ normalize(const object_descriptor_exprt &expr, const namespacet &ns) | |||
{ | |||
return expr; | |||
} | |||
if(expr.offset().id() == ID_unknown) | |||
if(!expr.offset().is_constant()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get a high level description of what is the normal form we're trying to reach ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To root object + constant offset, as object can be an arbitrary access path into the object. Pointer equality checks become trivial then - maybe simplify_expr has become good enough in the meanwhile.
Anyways, this seems orthogonal to this PR.
@@ -184,7 +183,7 @@ void value_sett::output(std::ostream &out, const std::string &indent) const | |||
stream << "<" << format(o) << ", "; | |||
|
|||
if(o_it->second) | |||
stream << *o_it->second; | |||
stream << format(*o_it->second); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now we have to print an expression instead of a mere integer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but why is that a concern?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have one remaining question: Now that we have symbolic offsets for pointer expressions instead of just "constants" or "unknown", is there ay way to use that to compute more precise results in get_value_set_rec
? I know it lets us be more precise when modelling assignments, but I don't understand why we don't have a similar gain in precision when computing dereferences/traversing value_sets.
Other question: now that the value set representation "knows" that an expression array[i]
has an offset of the form i * sizeof(T)
could we try to take into account extra constraints about i
during the value set traversal ? Let's say we're trying to resolve array[i]
in the context of a basic loop invariant 0 <= i && i <= len(array)
, knowing range constraints about i
we could maybe avoid injecting values representing OOB accesses in the value set for array[i]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems we are lacking tests in 4 places here. Given that this is all but trivial it would be great to find some test cases that trigger these.
@@ -981,7 +981,7 @@ normalize(const object_descriptor_exprt &expr, const namespacet &ns) | |||
{ | |||
return expr; | |||
} | |||
if(expr.offset().id() == ID_unknown) | |||
if(!expr.offset().is_constant()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To root object + constant offset, as object can be an arbitrary access path into the object. Pointer equality checks become trivial then - maybe simplify_expr has become good enough in the meanwhile.
Anyways, this seems orthogonal to this PR.
@@ -362,7 +361,8 @@ bool value_sett::eval_pointer_offset( | |||
if(!ptr_offset.has_value()) | |||
return false; | |||
|
|||
*ptr_offset += *it->second; | |||
*ptr_offset += |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codecov says that 361-368 are not covered by any tests.
if(!i.has_value()) | ||
i = mp_integer{0}; | ||
i = *i + *offset; | ||
additional_offset = plus_exprt{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codecov says that 731-733 are not covered by any tests.
} | ||
else | ||
{ | ||
*i *= *size; | ||
additional_offset = mult_exprt{ | ||
*additional_offset, from_integer(*size, additional_offset->type())}; | ||
|
||
if(expr.id()==ID_minus) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codecov says that 758-766 are not covered by any tests.
{ | ||
auto size = pointer_offset_size(array_type.element_type(), ns); | ||
|
||
if(!size.has_value() || *size == 0) | ||
o.reset(); | ||
else | ||
*o = *i * (*size); | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codecov says that 1416-1431 are not covered by any tests.
I have already seen this happen, although it isn't necessarily very obvious unless one starts examining the formula that symex produces. #8653 is a consequence of my observations: I was surprised to still find "unknown" when I had expected a known offset
I'm not sure we even do create those OOB values here? |
We can safely track arbitrary expressions as pointer offsets rather than limit ourselves to just constant offsets (and then treating all other expressions as "unknown").
41811ba
to
96844b5
Compare
We can safely track arbitrary expressions as pointer offsets rather than limit ourselves to just constant offsets (and then treating all other expressions as "unknown").